-
Notifications
You must be signed in to change notification settings - Fork 698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add XGBoost controller #1293
Add XGBoost controller #1293
Conversation
/hold |
adf5c9a
to
bd6a9c7
Compare
/cc @zw0610 please have a check |
@Jeffwan: GitHub didn't allow me to request PR reviews from the following users: please, have, a, check. Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
// satisfiedExpectations returns true if the required adds/dels for the given job have been observed. | ||
// Add/del counts are established by the controller at sync time, and updated as controllees are observed by the controller | ||
// manager. | ||
func (r *XGBoostJobReconciler) satisfiedExpectations(xgbJob *v1.XGBoostJob) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need expectations if we use controller-runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I am wrong. I think they are just helpers and help us save some efforts to determine if we need to resync job. Technically, I think we can definitely remove helpers if rest of the logics are solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
1. Move major controller logics from kubeflow/xgboost-operator to this repo. 2. Adapt to kubebuilder 3.0.0 change 3. Add xgboost train example 4. Leave some TODOs for code refactor later Signed-off-by: Jiaxin Shan <[email protected]>
bd6a9c7
to
684d36f
Compare
/cc @kubeflow/wg-training-leads |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
/hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jeffwan, terrytangyuan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
part of kubeflow/common#138 #1298 |
* Add XGBoost controller 1. Move major controller logics from kubeflow/xgboost-operator to this repo. 2. Adapt to kubebuilder 3.0.0 change 3. Add xgboost train example 4. Leave some TODOs for code refactor later Signed-off-by: Jiaxin Shan <[email protected]> * Move examples from config to example/ folder
Signed-off-by: Jiaxin Shan [email protected]
This is a working version. You can follow steps below